fix(test): replace Bun.sleep with utimes in dsn-cache mtime tests#612
Closed
betegon wants to merge 13 commits into
Closed
fix(test): replace Bun.sleep with utimes in dsn-cache mtime tests#612betegon wants to merge 13 commits into
betegon wants to merge 13 commits into
Conversation
Sleeping before a file write to force an mtime change is unreliable on Linux CI — the OS mtime resolution can be coarser than the delay, causing intermittent failures. Use utimes() instead to set the mtime to a guaranteed-different value, making the tests deterministic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛Test
Upgrade
Other
Documentation 📚
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The handler removal was inside if (client?.getOptions().enabled), so calling initSentry(false) in test afterEach hooks never cleaned up the handler registered by the prior initSentry(true) call. The stale handler kept the event loop alive after all tests finished, causing the bun process to hang indefinitely and the CI Unit Tests step to never complete. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…test Same flakiness as dsn-cache: 10ms sleep is unreliable on Linux CI with 1-second filesystem mtime resolution. Use utimes() to set an explicit future mtime instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary
All commands now require authentication by default. `buildCommand`
checks `isAuthenticated()` before invoking the command function and
throws `AuthError("not_authenticated")` if no token exists — this feeds
into the existing auto-auth middleware that prompts login and retries.
Previously each command had to manually guard auth or rely on an API
call failing deep in execution. `sentry init` in particular had no auth
check at all and would proceed into the wizard flow before eventually
failing.
## Changes
Added `auth?: boolean` to `buildCommand` options (defaults `true`). The
check runs before the generator, so the auto-auth middleware in `cli.ts`
catches it and kicks off the interactive login flow on unauthenticated
runs.
Commands that intentionally work without a token opt out with `auth:
false`:
- `auth login`, `auth logout`, `auth refresh`, `auth status`
- `help`, `schema`
- `cli fix`, `cli setup`, `cli upgrade`, `cli feedback`
With this in place, the redundant explicit `isAuthenticated()` checks in
`auth whoami` and `auth token` were removed.
## Test Plan
- `sentry init` (unauthenticated) → should immediately prompt
"Authentication required. Starting login flow..."
- `sentry auth login` (unauthenticated) → should work normally
- `sentry auth status` (unauthenticated) → should show "not
authenticated" cleanly, no login prompt
- `sentry auth logout` (unauthenticated) → should return "Not currently
authenticated."
- `SENTRY_AUTH_TOKEN=xxx sentry init` → no auth prompt, proceeds
normally
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| getAuthConfigSpy = spyOn(dbAuth, "getAuthConfig").mockReturnValue({ | ||
| token: "sntrys_test", | ||
| source: "config", | ||
| }); |
Contributor
There was a problem hiding this comment.
Mock uses invalid AuthSource value "config"
Low Severity
The getAuthConfig mock returns source: "config", but the AuthSource type only allows "env:SENTRY_AUTH_TOKEN", "env:SENTRY_TOKEN", or "oauth". Every other test file in the codebase that mocks getAuthConfig uses a valid source like "oauth". While the auth guard only checks for truthiness today, if any downstream code ever pattern-matches on source (e.g., checking for ENV_SOURCE_PREFIX), this mock would silently produce wrong test behavior.
This was referenced Mar 31, 2026
… stale beforeExit listeners
LightNodeClient.startClientReportTracking() and enableLogs both register
process.on('beforeExit', ...) handlers that are only removed by calling
client.close(). Without closing the previous client before Sentry.init(),
re-calling initSentry (e.g. initSentry(false) in test afterEach) accumulates
stale listeners that fire async work (HTTP sends) on beforeExit, preventing
the bun process from exiting after tests complete.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary Fixes the ~200 test failures caused by the auth guard added in #611. Meant to be merged into #612 so both the CI hang (dsn-cache timing + telemetry beforeExit) and the auth guard breakage ship together. ## Problem `buildCommand`'s new auth guard calls `getAuthConfig()` before every command. Test environments had no auth token set, so all command tests hit `AuthError("not_authenticated")` before the func body ran. ## Changes **Global fix (test/preload.ts):** - Set a fake `SENTRY_AUTH_TOKEN` in the test preload so the auth guard passes for all tests. Real API calls are blocked by the global fetch mock. **Framework tests (test/lib/command.test.ts):** - Add `auth: false` to all 29 test commands — these test flag handling, telemetry, and output rendering, not authentication. **Auth-specific tests (logout, refresh, whoami, project list):** - Tests that verify unauthenticated behavior or `SENTRY_TOKEN` priority now explicitly save/clear/restore `SENTRY_AUTH_TOKEN`. ## Test plan - 215 tests across the 7 most-affected files pass with 0 failures - `bun test test/commands` — 1209 pass - Lint and typecheck pass --------- Co-authored-by: Miguel Betegón <miguelbetegongarcia@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
… tests - log/list.test.ts: Mock resolveOrgProjectFromArg in 'allows --sort newest with --follow' test to prevent unmocked fetch call that triggered retry timeouts - wizard-runner.test.ts: Add default org option to skip resolveOrgSlug which calls listOrganizations API
betegon
added a commit
that referenced
this pull request
Mar 31, 2026
) ## Summary Fixes the CI unit test hang and ~200 test failures caused by three merged PRs (#610, #611). Supersedes #612 with a cleaner approach. ### 1. CI Hang — telemetry `beforeExit` handler leak `initSentry(false)` in test `afterEach` hooks never cleaned up the `beforeExit` handler registered by a prior `initSentry(true)` call — the removal was gated on `client?.getOptions().enabled`. SDK-internal handlers from `enableLogs` and `sendClientReports` also accumulated on re-init. **Fix** (src/lib/telemetry.ts): - Close previous Sentry client before re-init (`Sentry.getClient()?.close(0)`) - Move handler removal outside the `enabled` guard - Gate `enableLogs` and `sendClientReports` on `enabled` (not just `libraryMode`) - Snapshot and clean up `ProcessSession` `beforeExit` listeners in telemetry.test.ts ### 2. Auth guard test failures (PR #611) `buildCommand` now calls `getAuthConfig()` before every command. Tests had no auth token. **Fix**: - Set fake `SENTRY_AUTH_TOKEN` in test/preload.ts (blocked by global fetch mock) - Add `auth: false` to all 34 `buildCommand` calls in command.test.ts - Save/clear/restore `SENTRY_AUTH_TOKEN` in 9 test files that verify unauthenticated behavior - Add `getAuthConfig` mock + `resolveOrgProjectFromArg` mock in log/list tests ### 3. Dead code tests (PR #610) PR #610 moved org resolution and DSN detection from `createSentryProject` to `wizard-runner`'s `resolvePreSpinnerOptions`. Tests that exercised those flows through `handleLocalOp` hit the new `!options.org` guard. **Fix**: Rewrote 15 tests to call `resolveOrgSlug` and `detectExistingProject` directly, plus 2 new tests for `createSentryProject`'s existing-project check. No test coverage lost. ### 4. Mtime test flakiness Replaced `Bun.sleep(10)` with explicit `utimes()` calls in dsn-cache and project-root-cache tests for deterministic mtime differences on Linux CI. ## Test plan - `bun test` — 630 pass, 4 fail (pre-existing: 3 resolve-target timeouts from unmocked fetch, 1 log/list failure from missing generated api-schema.json) - `bun run lint` — clean (1 pre-existing warning) Made with [Cursor](https://cursor.com)
Remove the SENTRY_AUTH_TOKEN env var from test/preload.ts and replace it with explicit getAuthConfig mocks via a new useAuthMock() helper in test/helpers.ts. This is cleaner than the env var approach because: - Mocks are visible in each test file (no hidden global state) - Tests that need unauthenticated behavior work naturally - No env var save/delete/restore boilerplate needed in auth tests - Doesn't change the auth code path (env token vs DB token) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
There are 5 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| // Standard mode (project-scoped, no trace-id positional) | ||
| // ============================================================================ | ||
|
|
||
| useAuthMock(); |
Contributor
There was a problem hiding this comment.
Duplicate auth spies create brittle tests
Medium Severity
test/commands/log/list.test.ts now mocks dbAuth.getAuthConfig twice per test via both useAuthMock() and a manual beforeEach spy. This double wrapping can make mockRestore() ordering fragile and can fail in runners that disallow spying an already-spied method, causing intermittent or cascading test failures.
Additional Locations (1)
| token: "fake-token", | ||
| source: "oauth" as const, | ||
| }); | ||
| spyOn(auth, "isAuthenticated").mockReturnValue(true); |
Contributor
There was a problem hiding this comment.
Auth spies leak across wizard tests
Medium Severity
test/lib/init/wizard-runner.test.ts adds spyOn(auth, "getAuthConfig") and spyOn(auth, "isAuthenticated") in beforeEach but never restores them in afterEach. That leaves mocked auth state active between tests and can cause order-dependent failures or hidden regressions.
| // this, re-initializing the SDK (e.g., in tests) leaks setInterval handles | ||
| // that keep the event loop alive and prevent the process from exiting. | ||
| // close(0) removes listeners synchronously; we don't need to await the flush. | ||
| Sentry.getClient()?.close(0); |
Contributor
There was a problem hiding this comment.
Telemetry init closes host Sentry client
High Severity
initSentry() now calls Sentry.getClient()?.close(0) before every init. In library usage, this can close an already-initialized host application's Sentry client, not just this module's previous client, unexpectedly disabling host telemetry and mutating global process instrumentation.
| listener as (...args: unknown[]) => void | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Global listener cleanup can remove unrelated hooks
Medium Severity
The new afterAll removes every beforeExit listener not present at file load. In shared-process test execution, that can delete listeners registered by other suites during runtime, causing cross-test interference and nondeterministic failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test/lib/db/dsn-cache.test.tswere flaky on Linux CI: they calledBun.sleep(10)before writing a file, expecting the OS to advance the mtime. On Linux containers under load, mtime resolution can be coarser than the sleep delay, so the new and cached mtimes compared equal — making the cache appear valid when it shouldn't be.sleepapproach with explicitutimes()calls that set the mtime tocachedMtime + 5000ms, guaranteeing a detectable difference regardless of OS clock resolution or scheduler jitter.Test plan
test/lib/db/dsn-cache.test.tspass locally🤖 Generated with Claude Code